Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace invalid-UTF-8 smart quotes #372

Merged
merged 5 commits into from
Jun 29, 2016
Merged

Replace invalid-UTF-8 smart quotes #372

merged 5 commits into from
Jun 29, 2016

Conversation

lvh
Copy link
Contributor

@lvh lvh commented Jun 27, 2016

These bytes (0x92, 0x93), which appear to be intended as smart quotes, are invalid UTF-8, preventing it from being parsed by many tools. I have replaced them with single quotes (Unicode APOSTROPHE) to match the surrounding style. This is the only file with this defect.

Also cleaned up some end-of line whitespace.

I have found that most files do not end in newlines. Is that intentional?

Should you be interested in automated tooling that can help find this, here's the (fish, but will work in zsh and possibly recent bash) command I used to check that there was only one file:

iconv -f utf-8 -t utf-8 **json > /dev/null

I don't have a CLA; could you please send me in the right direction for submitting one?

@azurecla
Copy link

Hi @lvh, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

@lvh
Copy link
Contributor Author

lvh commented Jun 27, 2016

The build is failing on generating the compute client, which doesn't make a lot of sense to me. I could remove those commits, but it appears the build only looks at files in the PR, so that might simply silence the failure... Could someone else take a look to confirm? It looks like it might just be downloading the wrong Python package for codegen:

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/140436786#L250

@lmazuel
Copy link
Member

lmazuel commented Jun 27, 2016

Hi @lvh

The build breaks because of this bug:
#352

The current compute file is invalid, but no-one has produced a PR with the fix right now, so any PR with a compute modification will failed until someone fixes this bug.

If you have the opportunity to fix #352, it will be awesome :). You can push it here at the same time.

Thank you,

@lvh
Copy link
Contributor Author

lvh commented Jun 27, 2016

Thanks for the heads up @lmazuel. I thought the clients were generated from this Swagger. Is that not true, or has this caused a halt in the development of e.g. the Python Azure library?

@lmazuel
Copy link
Member

lmazuel commented Jun 27, 2016

@lvh We added a few new verifications of the Swagger validity and found a problem in the compute file. Since this new version of Autorest, the Python extension is no more able to generate the compute client.
Please note that @tbombach is currently working on a generic linting system and that at short term all clients (C#, Java, etc.) might be considered generated from an invalid file. If you have any way to push a fix of this bug, it would be great!
Thank you,

@lmazuel
Copy link
Member

lmazuel commented Jun 27, 2016

@lvh #359

@John-Hart
Copy link
Member

@lvh the Travis-ci issue has been fixed with #359 can you pull from upstream and push your changes again?

@lvh
Copy link
Contributor Author

lvh commented Jun 29, 2016

I'm working on getting the CLA signed; I have re-applied the fixes to compute on top of master.

@lvh lvh closed this Jun 29, 2016
@lvh lvh reopened this Jun 29, 2016
@azurecla
Copy link

Hi @lvh, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

@lvh
Copy link
Contributor Author

lvh commented Jun 29, 2016

(Oops; misclicked -- I did not intend to do that.)

@@ -116,7 +116,7 @@
"in": "query",
"required": false,
"type": "string",
"description": "The filter to apply on the operation. It ONLY supports the �eq� and �and� logical operators at this time. All the 4 query parameters 'OfferDurableId', 'Currency', 'Locale', 'Region' are required to be a part of the $filter."
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why the diff generator thinks eq changed but and didn't :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lvh It's showing both 'eq' and 'and' changed, is that not what you expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I changed both of the quotes. But the diff generator thinks the word itself changed, which it didn't. I checked in the commit diff; it's just a GitHub display issue.

@azurecla
Copy link

@lvh, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, AZPRBOT;

@John-Hart
Copy link
Member

Looks Good

@John-Hart John-Hart merged commit 59ac2af into Azure:master Jun 29, 2016
@AutorestCI
Copy link

No modification for NodeJS

@AutorestCI
Copy link

@AutorestCI
Copy link

No modification for Python

@AutorestCI
Copy link

No modification for NodeJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants